Conversation
plorenz
commented
Apr 2, 2026
plorenz
commented
- Add a doc about potential anycast support for links
- Add an initial doc exploring controller managed router configuration
- Updated with review comments and additions regarding collapsing the router types
- Update based on review feedback
- More updates from review changes
- Add a rough implementation plan
- Add note on config permissions
- Add target field to config types. Fixes Implement controller managed configuration for routers #3743
| string name = 2; | ||
| bytes schema = 3; | ||
| map<string, TagValue> tags = 4; | ||
| optional string target = 5; |
There was a problem hiding this comment.
Why optional? I assume this is for backward compatibility, but optional makes string be *string and doesn't really do much else besides letting us detect whether it was explicitly set or not. I don't see a reason to detect whether a setting is set or not.
I assume that the default is a target value of "service," and we could create an enum that does this:
enum ConfigTargetType {
CONFIG_TYPE_SERVICE = 0;
CONFIG_TYPE_ROUTER = 1;
}
Then the config type can be the enum type and default cleanly to 0 and not have to deal w/ pointer refs.
| 2. **Router data model.** Store fingerprints on the router data model and let them propagate | ||
| through the normal data model sync. Less chatty, since fingerprints only change on router | ||
| re-enrollment or cert rotation, not on every connect/disconnect. |
There was a problem hiding this comment.
I'd vote for RDM, there are other related changes that could be done, but don't solve this exact issue. They have trade-offs.
| @@ -0,0 +1,226 @@ | |||
| # Anycast Support for Router Links | |||
There was a problem hiding this comment.
Did you mean to include this on this PR? Seems out of place.
| t.AppendHeader(table.Row{"ID", "Name", "Target", "Schema"}) | ||
|
|
||
| for _, entity := range children { | ||
| wrapper := api.Wrap(entity) | ||
| t.AppendRow(table.Row{ | ||
| wrapper.String("id"), | ||
| wrapper.String("name"), | ||
| wrapper.String("target"), |
There was a problem hiding this comment.
Mismatched columns? 4 columns, only outputting 3 values?
| }, | ||
| Name: stringz.OrEmpty(configType.Name), | ||
| Name: stringz.OrEmpty(configType.Name), | ||
| Target: configType.Target, |
There was a problem hiding this comment.
Isn't this optional in the API def, and it isn't defaulted here? Who uses NIL set target types? The API should be clear, but if backward compatibility is the goal, it seems reasonable to default to "service".
| configType, _ := configTypeStore.LoadById(tx, config.TypeId) | ||
| configTypeName := "<not found>" | ||
| if configType != nil { | ||
| configTypeName = configType.Name | ||
| if configType.Target == nil || *configType.Target != db.ConfigTypeTargetService { | ||
| msg := fmt.Sprintf("config %v has config type %v which does not target services", | ||
| config.Name, configTypeName) | ||
| return errorz.NewFieldError(msg, "configs", entity.Configs) | ||
| } | ||
| } |
There was a problem hiding this comment.
So if it is nil, we don't validate it? So we allow nil values and don't validate their target type?
andrewpmartinez
left a comment
andrewpmartinez
left a comment
There was a problem hiding this comment.
I think overall, the PRs dealing with target type for configs need more effort to default to 'service' for backward compatibility. Allowing nil values and such is just a recipe for disaster.
2 participants
